[SymbolTable]: initial support for implicit declarations #746
Conversation
googlebot
added
the
cla: yes
ghost
force-pushed
the
symbol-table-implicit-declarations
branch
from
47eaf79 to
b9bbc81
Compare
Signed-off-by: Lukasz Dalek <[email protected]>
…ipsalliance#746 Signed-off-by: Lukasz Dalek <[email protected]>
| .resolved_symbol = &implicit_declaration, | ||
| }; | ||
|
|
||
| ref.PushReferenceComponent(implicit_ref); |
There was a problem hiding this comment.
Is this push needed? When a symbol is implicitly declared, do we ever need to build a hierarchical reference from it with the net/variable as a parent?
There was a problem hiding this comment.
I think we do. Implicit declaration is also an reference to that symbol and we want to keep that reference.
| const auto ref_map(module_m_info.LocalReferencesMapViewForTesting()); | ||
|
|
||
| ASSIGN_MUST_FIND(a_refs, ref_map, "a"); | ||
| ASSERT_EQ(a_refs.size(), 2); // all references to "N" parameter |
| const auto ref_map(module_m_info.LocalReferencesMapViewForTesting()); | ||
|
|
||
| ASSIGN_MUST_FIND(a_refs, ref_map, "a"); | ||
| ASSERT_EQ(a_refs.size(), 1); // all references to "N" parameter |
| const SymbolTableNode& root_symbol(symbol_table.Root()); | ||
|
|
||
| const auto build_diagnostics = BuildSymbolTable(src, &symbol_table); | ||
| EXPECT_EMPTY_STATUSES(build_diagnostics); |
There was a problem hiding this comment.
Should we expect a diagnostic on an attempt to re-define a in the same scope?
There was a problem hiding this comment.
We do have such diagnostic (test case covering that is here: https://github.com/google/verible/pull/746/files#diff-45eb889cefc594107d8a95e50aa16ed073b4a7bf296ea244e2c20daa9762fe9cR2558)
I think that you meant an attempt to reference an implictly declared variable.
I thought about that and I think it would be nice to have an extra flag/switch that would report such references but I think that exceeds scope of this PR.
There was a problem hiding this comment.
Sure, in the (near?) future, we should issue some diagnostic during Build() the way we do in other cases for declarations.
What should happen during Resolve() in this example when there is a reference to a? Can we note the intended behavior with an additional test case?
Signed-off-by: Lukasz Dalek <[email protected]>
ghost
force-pushed
the
symbol-table-implicit-declarations
branch
from
b9bbc81 to
1b97e86
Compare
Signed-off-by: Lukasz Dalek <[email protected]>
Signed-off-by: Lukasz Dalek <[email protected]>
| @@ -161,6 +161,9 @@ static absl::Status DiagnoseMemberSymbolResolutionFailure( | |||
| context_name, ".")); | |||
| } | |||
|
|
|||
| static const SymbolTableNode* LookupSymbolUpwards( | |||
There was a problem hiding this comment.
If you like, you may move the below definition earlier in the file.
| .resolved_symbol = &implicit_declaration, | ||
| }; | ||
|
|
||
| ref.PushReferenceComponent(implicit_ref); |
| const SymbolTableNode& root_symbol(symbol_table.Root()); | ||
|
|
||
| const auto build_diagnostics = BuildSymbolTable(src, &symbol_table); | ||
| EXPECT_EMPTY_STATUSES(build_diagnostics); |
There was a problem hiding this comment.
Sure, in the (near?) future, we should issue some diagnostic during Build() the way we do in other cases for declarations.
What should happen during Resolve() in this example when there is a reference to a? Can we note the intended behavior with an additional test case?
| for (const auto& n_a_ref : n_a_refs) { | ||
| const ReferenceComponent& n_a_ref_comp(n_a_ref->components->Value()); | ||
| EXPECT_EQ(n_a_ref_comp.resolved_symbol, | ||
| &a_variable); // resolved successfully |
There was a problem hiding this comment.
I might need a language lawyer to explain to me. Is the second declaration of "a" in the nested module supposed to resolve to that in the outer module? Maybe add a comment to this test case that cites the LRM passage about this.
Support implicit declarations as defined in SystemVerilog LRM
6.10: Implicit declarations